Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Drop hsa prefix from include directories so users can #include <hsa/*> #112

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Drop hsa prefix from include directories so users can #include <hsa/*> #112

wants to merge 1 commit into from

Conversation

haampie
Copy link

@haampie haampie commented Jan 11, 2021 •
edited
Loading

It should really be preferred to use

#include <hsa/hsa.h>

over

#include <hsa.h>

and it seems downstream packages such as HIP are simply not using the hsa-runtime64::hsa-runtime64 target because it does not include the hsa/ prefix (?)

@haampie
Copy link
Author

haampie commented Jan 15, 2021

@skeelyamd does this repo accept PRs?

@skeelyamd
Copy link
Collaborator

It does and this PR is being considered, apologies for the silence. Ultimately the path to include is not up to me though. Another team must approve packaging, include, and directory layout changes. We've been trying to get to a flat include directory structure so that the packages can be configured for distro standard install paths more easily for a while. That process halted part way through to let other components start to use cmake targets. HIP should be using cmake targets at this point though they may not be using them completely correctly. Using the target is required for static builds and HIP does support this now. It seems it's time to resume this process.

There seems to be conflicting best practices on how a cmake project's include paths should be reported. Most that I've seen do not require a project specific path prefix as you suggest here. But there are notable exceptions. Is there a reference supporting this patch that should be considered? Changing the prefix breaks backward compatibility with existing code so is not something that we should do lightly.

@haampie
Copy link
Author

haampie commented Jan 15, 2021 •
edited
Loading

We've been trying to get to a flat include directory structure so that the packages can be configured for distro standard install paths more easily for a while. That process halted part way through to let other components start to use cmake targets.

I think this is a good decision; among other things not all package managers support flat directory structures either (e.g. spack).

Is there a reference supporting this patch that should be considered?

https://cliutils.gitlab.io/modern-cmake/chapters/basics/structure.html is quite nice

Changing the prefix breaks backward compatibility with existing code so is not something that we should do lightly.

True, but there's many instances where scripts use their own find logic to detect hsa.h, and they use different includes. From spack stage $(spack dependents hsa-rocr-dev) and some grepping:

opencl-on-vdi/amdocl/cmake/modules/FindROCR.cmake                   :find_path(FIND_ROCR_INCLUDES hsa.h HINTS /opt/rocm/include /opt/rocm/hsa/include PATH_SUFFIXES hsa)
roctracer-dev/cmake_modules/env.cmake                               :find_file ( HSA_RUNTIME_INC "hsa/hsa.h" )
hip-4.0.0/hip-config.cmake.in                                       :find_path(HSA_HEADER hsa/hsa.h
hip-4.0.0/cmake/FindROCR.cmake                                      :find_path(FIND_ROCR_INCLUDES hsa.h HINTS /opt/rocm/include /opt/rocm/hsa/include PATH_SUFFIXES hsa)
rocm-debug-agent-4.0.0/cmake/modules/FindROCR.cmake                 :find_path(FIND_ROCR_INCLUDES hsa/hsa.h HINTS /opt/rocm/include /opt/rocm/hsa/include)
rocm-opencl-4.0.0/amdocl/cmake/modules/FindROCR.cmake               :find_path(FIND_ROCR_INCLUDES hsa.h HINTS /opt/rocm/include /opt/rocm/hsa/include PATH_SUFFIXES hsa)
rocprofiler-dev-4.0.0/roctracer/cmake_modules/env.cmake             :find_file ( HSA_RUNTIME_INC "hsa/hsa.h" )
rocprofiler-dev-4.0.0/cmake_modules/env.cmake                       :find_file ( HSA_RUNTIME_INC "hsa/hsa.h" )
roctracer-dev-4.0.0/cmake_modules/env.cmake                         :find_file ( HSA_RUNTIME_INC "hsa/hsa.h" )
hip-rocclr-4.0.0/cmake/modules/FindROCR.cmake                       :find_path(FIND_ROCR_INCLUDES hsa.h HINTS /opt/rocm/include /opt/rocm/hsa/include PATH_SUFFIXES hsa)
hip-rocclr-4.0.0/opencl-on-vdi/amdocl/cmake/modules/FindROCR.cmake  :find_path(FIND_ROCR_INCLUDES hsa.h HINTS /opt/rocm/include /opt/rocm/hsa/include PATH_SUFFIXES hsa)
atmi-4.0.0/src/cmake_modules/FindROCm.cmake                         :    hsa.h

these try to find hsa.h themselves and do not use the target. Some use this target

amd-llvm-project/openmp/libomptarget/plugins/hsa/CMakeLists.txt                      :  hsa-runtime64::hsa-runtime64
aomp-3.10.0/aomp-dir/amd-llvm-project/openmp/libomptarget/plugins/hsa/CMakeLists.txt :  hsa-runtime64::hsa-runtime64
hip-4.0.0/lpl_ca/CMakeLists.txt                                                      :target_link_libraries(ca PUBLIC hsa-runtime64::hsa-runtime64 )
hip-4.0.0/rocclr/CMakeLists.txt                                                      :    target_link_libraries(amdhip64 PRIVATE amdrocclr_static Threads::Threads dl hsa-runtime64::hsa-runtime64)
hip-4.0.0/rocclr/CMakeLists.txt                                                      :    target_link_libraries(amdhip64 PRIVATE Threads::Threads dl hsa-runtime64::hsa-runtime64 amd_comgr)
hip-rocclr-4.0.0/CMakeLists.txt                                                      :  target_link_libraries(amdrocclr_static PRIVATE hsa-runtime64::hsa-runtime64)
hip-rocclr-4.0.0/device/rocm/CMakeLists.txt                                          :    $<TARGET_PROPERTY:hsa-runtime64::hsa-runtime64,INTERFACE_INCLUDE_DIRECTORIES>)
rccl-4.0.0/test/CMakeLists.txt                                                       :    target_link_libraries(UnitTests PRIVATE amdhip64 amd_comgr hsa-runtime64::hsa-runtime64)
rocm-bandwidth-test-4.0.0/CMakeLists.txt                                             :   target_link_libraries(${TEST_NAME} PRIVATE hsa-runtime64::hsa-runtime64)
rocminfo-4.0.0/CMakeLists.txt                                                        :target_link_libraries(${ROCMINFO_EXE} hsa-runtime64::hsa-runtime64)

and in terms of includes of hsa/hsa.h:

aomp-3.10.0/examples/cloc/vector_copy/vector_copy.cpp                                                       :#include <hsa/hsa.h>
hip-4.0.0/include/hip/hcc_detail/hip_runtime_api.h                                                          :#include <hsa/hsa.h>
hip-4.0.0/include/hip/hcc_detail/hsa_helpers.hpp                                                            :#include <hsa/hsa.h>
hip-4.0.0/include/hip/hcc_detail/program_state.hpp                                                          :#include <hsa/hsa.h>
hip-4.0.0/src/code_object_bundle.inl                                                                        :#include <hsa/hsa.h>
hip-4.0.0/src/hip_hcc_internal.h                                                                            :#include <hsa/hsa.h>
hip-4.0.0/src/hip_memory.cpp                                                                                :#include "hsa/hsa.h"
hip-4.0.0/src/hip_module.cpp                                                                                :#include <hsa/hsa.h>
hip-4.0.0/src/hip_texture.cpp                                                                               :#include "hsa/hsa.h"
hip-4.0.0/src/hiprtc.cpp                                                                                    :#include <hsa/hsa.h>
hip-4.0.0/src/program_state.cpp                                                                             :#include <hsa/hsa.h>
hip-4.0.0/src/program_state.inl                                                                             :#include <hsa/hsa.h>
llvm-project/openmp/libomptarget/plugins/hsa/impl/hostcall.cpp                                              :#include <hsa/hsa.h>
rccl-4.0.0/src/graph/topo.cc                                                                                :#include <hsa/hsa.h>
rccl-4.0.0/src/graph/xml.cc                                                                                 :#include <hsa/hsa.h>
rccl-4.0.0/src/graph/xml.cc                                                                                 :#include <hsa/hsa.h>
rccl-4.0.0/src/transport/p2p.cc                                                                             :#include <hsa/hsa.h>
rocm-dbgapi-4.0.0/src/dispatch.h                                                                            :#include <hsa/hsa.h>
rocm-dbgapi-4.0.0/src/queue.cpp                                                                             :#include <hsa/hsa.h>
rocm-debug-agent-4.0.0/src/debug_agent.cpp                                                                  :#include <hsa/hsa.h>
rocm-openmp-extras-4.0.0/examples/cloc/vector_copy/vector_copy.cpp                                          :#include <hsa/hsa.h>
rocm-openmp-extras-4.0.0/rocm-openmp-extras/llvm-project/openmp/libomptarget/plugins/hsa/impl/hostcall.cpp  :#include <hsa/hsa.h>
rocm-validation-suite-4.0.0/pebb.so/include/worker_b2b.h                                                    :#include "hsa/hsa.h"
rocm-validation-suite-4.0.0/pebb.so/src/action_run.cpp                                                      :#include "hsa/hsa.h"
rocm-validation-suite-4.0.0/pebb.so/src/action.cpp                                                          :#include "hsa/hsa.h"
rocm-validation-suite-4.0.0/pqt.so/include/action.h                                                         :#include "hsa/hsa.h"
rocm-validation-suite-4.0.0/pqt.so/include/worker_b2b.h                                                     :#include "hsa/hsa.h"
rocm-validation-suite-4.0.0/src/rvshsa.cpp                                                                  :#include "hsa/hsa.h"
rocprofiler-dev-4.0.0/roctracer/inc/ext/hsa_rt_utils.hpp                                                    :#include <hsa/hsa.h>
roctracer-dev-4.0.0/inc/ext/hsa_rt_utils.hpp                                                                :#include <hsa/hsa.h>
roctracer-dev/inc/ext/hsa_rt_utils.hpp                                                                      :#include <hsa/hsa.h>

@keryell
Copy link

keryell commented Nov 17, 2022

Any progress on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@haampie @skeelyamd @keryell